Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: IncrementCapturerCount doesn't increase the capturer count #32973

Merged
merged 1 commit into from Mar 21, 2022
Merged

fix: IncrementCapturerCount doesn't increase the capturer count #32973

merged 1 commit into from Mar 21, 2022

Conversation

zeeker999
Copy link
Contributor

@zeeker999 zeeker999 commented Feb 18, 2022 •
edited

Description of Change

This regression was introduced by commit 22a70eb. The change done by WebContentsImpl::IncrementCapturerCount() is reverted just before returning in WebContents::IncrementCapturerCount(), so the isBeingCaptured() always returns false. Fixed the issue by not running the close closure returned by web_contents::IncrementCapturerCount().

Please double-check that this change is fine since I'm not familiar with this code.

cc @ckerr @codebytere @deepak1556 @MarshallOfSound

Related #30666

Affected branches: 13-x-y to the current stable branch, please consider backporting this fix to them.

base::ScopedClosureRunner WebContentsImpl::IncrementCapturerCount(
    const gfx::Size& capture_size,
    bool stay_hidden,
    bool stay_awake) {`
  ....
  return base::ScopedClosureRunner(
      base::BindOnce(&WebContentsImpl::DecrementCapturerCount,
                     weak_factory_.GetWeakPtr(), stay_hidden, stay_awake));
}

Checklist

Release Notes

Notes: Fix the IncrementCapturerCount regression introduced by 13.0.0-beta.21

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 18, 2022
@deepak1556
Copy link
Member

deepak1556 commented Feb 18, 2022 •
edited

Thanks for working on this fix, the upstream change https://chromium-review.googlesource.com/c/chromium/src/+/2807829 makes it such the callers of IncrementCapturerCount should hold onto the capture handle until the capturing is done and the handle eventually resolves to invoking DecrementCapturerCount internally. So there are couple of things we can change to align with this behavior,

  1. Remove https://github.com/electron/electron/blob/main/patches/chromium/fix_expose_decrementcapturercount_in_web_contents_impl.patch in favor of capture handle. Basically we should store the handle received from call to IncrementCapturerCount and api::WebContents::DecrementCaptureCount should just RunAndReset the stored capture handle instead of calling the private api content::WebContents::DecrementCapturerCount.

  2. Deprecate contents.decrementCaptureCount api in favor of destroying the capture handle automatically when a corresponding capture page action is completed via OnCapturePageDone

@zeeker999
Copy link
Contributor Author

zeeker999 commented Feb 18, 2022 •
edited

...

  1. Remove https://github.com/electron/electron/blob/main/patches/chromium/fix_expose_decrementcapturercount_in_web_contents_impl.patch in favor of capture handle. Basically we should store the handle received from call to IncrementCapturerCount and api::WebContents::DecrementCaptureCount should just reset the stored capture handle instead of calling the private api content::WebContents::DecrementCapturerCount.

I don't think it's feasible since the IncrementCapturerCount could be nested.

  1. Deprecate contents.decrementCaptureCount api in favor of destroying the capture handle automatically when a corresponding capture page action is completed via OnCapturePageDone

While this could work, it isn't the perfect approach. The thing I want to do is

  • IncrementCapturerCount() to make keep the page visible
  • capturePage() periodically to take screenshots of the page
  • decrementCaptureCount() when the work is done.

@zeeker999
Copy link
Contributor Author

zeeker999 commented Feb 18, 2022 •
edited

Or do you consider merging the request first then working on a new approach for future releases? For example, wrap the handler and return it to the caller, let javascript run the callback sometime later, and remove the decrementCaptureCount.

@deepak1556
Copy link
Member

For example, wrap the handler and return it to the caller, let javascript run the callback anytime later, and remove the decrementCaptureCount.

Yeah that sounds like a good way forward.

@deepak1556 deepak1556 added semver/patch backwards-compatible bug fixes target/16-x-y labels Feb 18, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 19, 2022
@zcbenz zcbenz merged commit 108ee70 into electron:main Mar 21, 2022
@release-clerk
Copy link

release-clerk bot commented Mar 21, 2022

Release Notes Persisted

Fix the IncrementCapturerCount regression introduced by 13.0.0-beta.21

@trop trop bot mentioned this pull request Mar 21, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Mar 21, 2022

I was unable to backport this PR to "15-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Mar 21, 2022

I was unable to backport this PR to "16-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/15-x-y label Mar 21, 2022
@trop
Copy link
Contributor

trop bot commented Mar 21, 2022

I was unable to backport this PR to "17-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Mar 21, 2022

I have automatically backported this PR to "18-x-y", please check out #33371

@zeeker999 zeeker999 mentioned this pull request Mar 24, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Mar 24, 2022

@zeeker999 has manually backported this PR to "17-x-y", please check out #33430

bavulapati pushed a commit to bavulapati/electron that referenced this pull request Apr 29, 2022
@zeeker999 @bavulapati
fix: IncrementCapturerCount doesn't increase the capturer count (elec…
ed69e59 
…tron#32973)
@commit-lint commit-lint bot mentioned this pull request May 9, 2022
Open
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@zeeker999 @khalwa
fix: IncrementCapturerCount doesn't increase the capturer count (elec…
cf62710 
…tron#32973)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zeeker999 @deepak1556 @zcbenz @codebytere @miniak